Fix phpstan/phpstan#14250: trait.duplicateMethod is not reported#5181
Fix phpstan/phpstan#14250: trait.duplicateMethod is not reported#5181phpstan-bot wants to merge 8 commits intophpstan:2.1.xfrom
Conversation
… methods in traits - Created DuplicateTraitDeclarationRule that listens for InTraitNode to detect duplicate methods, properties, and constants within trait declarations - The existing DuplicateDeclarationRule only handles InClassNode, which is never emitted for traits (NodeScopeResolver skips trait declarations at line 967-968) - InTraitNode is emitted when a trait is used by a class, allowing the new rule to check trait bodies for duplicates - Added regression test in tests/PHPStan/Rules/Classes/data/bug-14250.php
VincentLanglet
left a comment
There was a problem hiding this comment.
This could be refactor with DuplicateDeclarationRule by creating some DuplicateDeclarationHelper
Also, this need more tests, for each possible trait duplicate content
…clarationRule and DuplicateTraitDeclarationRule Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ts, properties, promoted properties, methods) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. Here's what I did to address the review: 1. Extracted 2. Refactored both rules to use the helper:
3. Added comprehensive tests for each type of duplicate content in traits:
|
|
Static analysis passes. The one test failure ( The review from VincentLanglet is an approval with no additional changes requested. The previous feedback (extract helper, add more tests) was already addressed in the latest commits. Everything looks good — no further action needed. |
| class Qux | ||
| { | ||
| use TraitWithDuplicateMethods; | ||
| } |
There was a problem hiding this comment.
append the following case in this test-file and add expected errors:
trait MyTrait1
{
public function doSomething(): void
{
}
}
trait MyTrait2
{
public function doSomething(): void
{
}
}
class FooWithMulitpleConflictingTraits
{
use MyTrait1, MyTrait2;
}
There was a problem hiding this comment.
Done. Added the test case with two separate traits (MyTrait1 and MyTrait2) each having a doSomething() method, used together by FooWithMultipleConflictingTraits. Since neither trait has duplicate declarations within itself, the DuplicateTraitDeclarationRule correctly reports no errors for this case (no false positives). The commit has been pushed.
There was a problem hiding this comment.
we are missing errors when a class uses multiple traits with conflicting symbols
There was a problem hiding this comment.
Pushed. Here's what I did to address the review:
Added cross-trait method collision detection — When a class uses multiple traits that define the same method without insteadof resolution, DuplicateDeclarationRule now reports an error. This mirrors PHP's fatal error: "Trait method X::method() has not been applied, because there are collisions with other trait methods on ClassName."
Changes:
DuplicateDeclarationHelper: AddedcheckTraitMethodCollisions()method that collects methods from each trait used by a class, respectsinsteadofadaptations and class-level method overrides, and reports unresolved conflictsDuplicateDeclarationRule: Now callscheckTraitMethodCollisions()in addition tocheckClassLike()DuplicateDeclarationRuleTest: AddedtestBug14250TraitMethodCollisions(verifies the multi-trait case frombug-14250.php) andtestTraitMethodCollisions(comprehensive test with unresolved collisions,insteadofresolution, class overrides, single trait, and partial collisions)
All tests pass (except the pre-existing RawErrorFormatterTest failure unrelated to this PR). Static analysis passes with no errors.
There was a problem hiding this comment.
Also add a test where the method is renamed when the trait is imported or the conflict is already resolved.
trait A {
public function smallTalk() {
echo 'a';
}
public function bigTalk() {
echo 'A';
}
}
trait B {
public function smallTalk() {
echo 'b';
}
public function bigTalk() {
echo 'B';
}
}
class Talker {
use A, B {
B::smallTalk insteadof A;
A::bigTalk insteadof B;
}
}
class Aliased_Talker {
use A, B {
B::smallTalk insteadof A;
A::bigTalk insteadof B;
B::bigTalk as talk;
}
}
No error should be reported
There was a problem hiding this comment.
The background PHPStan task completed successfully (already confirmed above — no errors). Everything is pushed and ready.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When a class uses multiple traits that define the same method without `insteadof` resolution, PHP reports a fatal error. Add detection for this case in DuplicateDeclarationRule via a new checkTraitMethodCollisions helper method. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nd as alias Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
VincentLanglet
left a comment
There was a problem hiding this comment.
Fix the lint build
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------------
24 | ERROR | [x] Class \PHPStan\Rules\IdentifierRuleError should not be
| | referenced via a fully qualified name, but via a use
| | statement.
| | (SlevomatCodingStandard.Namespaces.ReferenceUsedNamesOnly.ReferenceViaFullyQualifiedName)
127 | ERROR | [x] Class \PHPStan\Rules\IdentifierRuleError should not be
| | referenced via a fully qualified name, but via a use
| | statement.
| | (SlevomatCodingStandard.Namespaces.ReferenceUsedNamesOnly.ReferenceViaFullyQualifiedName)
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------
…eference Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Fixed the lint errors by adding |
|
The review from VincentLanglet is an approval with no additional changes requested. All previous feedback (extract helper, add more tests, fix lint, add insteadof/alias tests) has already been addressed in the existing commits. No further action is needed. |
Summary
Duplicate method declarations within traits were not being reported with the
trait.duplicateMethodidentifier. The existingDuplicateDeclarationRulelistens forInClassNode, butInClassNodeis never emitted for trait declarations —NodeScopeResolverskips trait nodes entirely at declaration time (they are only processed whenused by a class).Changes
src/Rules/Classes/DuplicateTraitDeclarationRule.php— a new rule that implementsRule<InTraitNode>and checks for duplicate methods, properties, and constants within trait declarationsDuplicateDeclarationRulebut operates onInTraitNode(emitted when a trait is used by a class)#[RegisteredRule(level: 0)], same as the original ruletests/PHPStan/Rules/Classes/DuplicateTraitDeclarationRuleTest.phptests/PHPStan/Rules/Classes/data/bug-14250.phpRoot cause
NodeScopeResolver::processStmtNodes()short-circuits trait declarations (line 967-968), returning immediately without emittingInClassNode. This is by design — traits are processed in the context of their using class viaprocessTraitUse(). However,InTraitNodeIS emitted during trait use processing, so a rule listening forInTraitNodecan detect duplicate declarations within traits. No such rule existed before this fix.Test
The regression test in
bug-14250.phpdefines a trait with two methods nameddoSomething()and a class that uses the trait. The test expects one error: "Cannot redeclare method Bug14250\MyTrait::doSomething()." at line 11.Fixes phpstan/phpstan#14250